test: Add integration test for /var mounts #6033
Conversation
There was a problem hiding this comment.
Thanks @TheRealFalcon, this is looking really nice! I left some nitpicks below.
| crit_chain = client.execute( | ||
| f"systemd-analyze critical-chain {service} --fuzz=60" | ||
| ) | ||
| assert "var.mount" in crit_chain |
There was a problem hiding this comment.
Could we add a failure message here? Currently when this fails we see:
assert 'var.mount' in 'The time when unit became active or started is printed after the "@" character.\nThe time the unit took to start is printed after the "+" character.\n\ncloud-init-...
Which is a bit cryptic without context
| f"systemd-analyze critical-chain {service} --fuzz=60" | ||
| ) | ||
| assert "var.mount" in crit_chain | ||
| assert crit_chain.find("var.mount") > crit_chain.find(service) |
There was a problem hiding this comment.
It would be nice if it offered a machine readable output format, but that doesn't seem to be the case (yet?).
If the service isn't available on in the critical chain, we would get a false positive (PASS) on this test. There would be bigger issues at play, but for easier maintenance I think that failing in that case would be more correct.
That said, I think that systemctl list-dependencies --all {service} would be more correct than using the critical chain, right?
Also, same comment as above about explaining why the failure matters.
Would you mind checking cloud-init logs/status here too, just for good measure?
There was a problem hiding this comment.
I'm not sure why we'd get a false positive, but I think systemctl makes more sense than some arbitrary fuzz value so I'll do that. +1 to everything else here too
There was a problem hiding this comment.
I'm not sure why we'd get a false positive
str.find(sub[, start[, end]])
...
Return -1 if sub is not found.
-1 would be smaller than the integer returned by crit_chain.find("var.mount")
TheRealFalcon
force-pushed
the
var-test
branch
from
da23e18 to
d2e19db
Compare
|
@holmanb I believe I addressed your comments |
Proposed Commit Message
Additional Context
Test Steps
Merge type